Repository navigation
Signal each class of posterior warning once - #416
florence-bockting wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #416 +/- ##
==========================================
+ Coverage 92.65% 92.69% +0.04%
==========================================
Files 31 31
Lines 3240 3258 +18
==========================================
+ Hits 3002 3020 +18
Misses 238 238 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 45a7c81 is merged into master:
|
jgabry
left a comment
jgabry
left a comment
There was a problem hiding this comment.
This looks good. I made a few small suggestions.
Also one design question: before this PR users would see something like
There were 20 warnings (use warnings() to see them)"
and now with this PR they just get a single warning with no count. For something like ESS capping that's probably good, but for something like whether 1 observation or 900 observations get a Pareto k warning it could matter. Should we add something like "(n times)" to the message? I'm not sure. Maybe @avehtari and/or @VisruthSK have thoughts on this too.
| .collect_warnings <- function(expr) { | ||
| collected <- list() | ||
| out <- withCallingHandlers( | ||
| expr, | ||
| posterior_warning = function(cnd) { | ||
| collected[[class(cnd)[1]]] <<- cnd | ||
| invokeRestart("muffleWarning") | ||
| } | ||
| ) | ||
| for (cnd in collected) { | ||
| warning(cnd) | ||
| } | ||
| out | ||
| } |
There was a problem hiding this comment.
I think if the function the user calls errors for whatever reason they won't see the warnings. What about this slightly modified version that moves the loop into an on.exit. I think this would allow the user to see the warnings even after an error, although I haven't tested it.
.collect_warnings <- function(expr) {
collected <- list()
on.exit(for (cnd in collected) warning(cnd))
withCallingHandlers(
expr,
posterior_warning = function(cnd) {
collected[[class(cnd)[1]]] <<- cnd
invokeRestart("muffleWarning")
}
)
}| options(loo.cores = NULL) | ||
| }) | ||
|
|
||
| test_that(".collect_warnings() signals each posterior class once", { |
There was a problem hiding this comment.
With CRAN posterior 1.7.1 there are no warning classes yet, so in CI no posterior warning ever gets to .collect_warnings() from relative_eff(), E_loo(), or loo_moment_match().
I think this test for the helper itself is good and we should keep it, but maybe add some tests that the functions users call actually signal each warning only once?
How about adding these two tests?
- One that calls the real loo function but with posterior mocked, e.g.
local_mocked_bindings(ess_mean = ..., .package = "posterior"), with a mock that signals a classed warning, so it runs in CI now. (local_mocked_bindings()needs testthat >= 3.1.7, so we'd need to bump that in DESCRIPTION.) - One that uses the real posterior functions without mocking, but we skip it until the next posterior release is available, e.g.
skip_if_not(packageVersion("posterior") > "1.7.1").
| out <- withCallingHandlers( | ||
| expr, | ||
| posterior_warning = function(cnd) { | ||
| collected[[class(cnd)[1]]] <<- cnd |
There was a problem hiding this comment.
I think this keeps the last warning of each class, not the first. The only class this really affects is pareto_tail_exceeds_draws, since those messages include actual numbers. I'm not sure that this really matters, though. What do you think?
| * Signal each class of posterior warning once per call in `relative_eff()`, | ||
| `E_loo()`, and `loo_moment_match()` by @florence-bockting in #416 |
There was a problem hiding this comment.
Right now this only applies if the user has the GitHub version of posterior. Should we mention that it's conditional on posterior > 1.7.1?
Description
This PR is connected to stan-dev/posterior#470 where we introduced warning classes.
These can now be extracted and collected through the new
.collect_warnings()function in loo.This collector ensures that the same warning is printed only once (and not multiple times when e.g. running some posterior function in a loop).
This PR is related to Issue #353 (but it does not solve it entirely).
TODOs
The PR was assisted by an LLM.